fix: correct grape/sraps API library issues found in doc review#51
Merged
fix: correct grape/sraps API library issues found in doc review#51
Conversation
Parse APIError responses to map permission errors (taken_mac_or_forbidden, User lacks permission to endpoint) to the semantic error code device_owned_by_other_user. This improves error clarity for clients when a MAC address is registered in another account. Changes: - Add pattern matching in ParseProviderError for grape/sraps permission errors - Implement error parsing in both GrapeDevice and SrapsDevice Register methods - Call ParseProviderError on APIError to get semantic error codes Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The error message was hardcoded to 'GRAPE API error (HTTP X): ...' even when the library is used by the SRAPS provider, making error messages misleading in that context. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The field was serialized as null, which on a full-update PUT explicitly resets the server-side default (3 months) for all registered devices. Adding omitempty ensures the field is omitted when no value is set, leaving the server default intact. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The GRAPE/SRAPS settings API paginates with a default page size of 20 and a maximum of 1000. With 1179 global settings, fetching only the first page could silently miss the provisioning setting UUID, causing all registrations to fail on accounts where the setting appears beyond page 1. Changes: - Add makeHawkRequestFull() that returns both body and response headers alongside the existing makeHawkRequest() wrapper. - Rewrite getProvisioningServerID() to follow RFC 5988 Link headers (rel="next") until the setting is found or pages are exhausted. - Use page_size=1000 on the initial request to minimise round-trips. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes four correctness issues in the Grape/SRAPS provider library, discovered during an API documentation review: mapping permission errors to semantic codes, making error message prefixes provider-neutral, preventing null from resetting a server-side default via omitempty, and paginating the settings list to find the provisioning setting UUID across all pages.
Changes:
- Parse
APIErrorresponses in both providers to map permission-related errors todevice_owned_by_other_user, and rename the error prefix from "GRAPE API error" to "API error" - Add
omitemptytoWarrantyExpWarningPeriodto avoid sendingnullon PUT requests, and add pagination support (parseLinkNext+makeHawkRequestFull) for the settings endpoint - Add new grape/sraps error patterns (
taken_mac_or_forbidden, permission+endpoint) toParseProviderError
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| utils/utils.go | Add two new grape/sraps error message patterns to map to device_owned_by_other_user |
| providers/grape.go | Use ParseProviderError to convert API errors to semantic error codes instead of always returning provider_remote_call_failed |
| providers/sraps.go | Same ParseProviderError integration as grape.go |
| libs/grape/types.go | Add omitempty to WarrantyExpWarningPeriod JSON tag to avoid sending null |
| libs/grape/errors.go | Rename "GRAPE API error" prefix to generic "API error" |
| libs/grape/devices.go | Add paginated settings lookup with page_size=1000 and RFC 5988 Link header parsing |
| libs/grape/auth.go | Introduce makeHawkRequestFull returning headers alongside body; refactor makeHawkRequest as a thin wrapper |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Remove dead type-assertion blocks after ParseProviderError call. All three branches returned parsedErr unchanged; the *models.ProviderError pointer assertion can never match since ParseProviderError never returns a pointer. Simplify to a direct return. Review suggestions: - #51 (comment) (grape.go) - #51 (comment) (sraps.go) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request introduces improvements to the Hawk-authenticated API request handling, enhances pagination support for settings retrieval, refines error handling and messaging, and improves semantic error mapping for device registration. The most important changes are grouped below by theme.
API Request Handling & Pagination
makeHawkRequestFull, which returns both the response body and headers to support Link-based pagination. The originalmakeHawkRequestnow delegates to this new method. [1] [2] [3] [4]getProvisioningServerIDto handle paginated responses using the Link header, ensuring all pages are fetched and searched for the provisioning setting UUID. Added theparseLinkNexthelper for extracting the next page URL.Error Handling & Messaging
"device_owned_by_other_user", instead of generic errors. This affects bothGrapeDeviceandSrapsDeviceregistration. [1] [2]ParseProviderErrorto recognize additional error messages specific to grape/sraps, including"taken_mac_or_forbidden"and permission-related endpoint errors.APIErrormessage formatting to remove the "GRAPE" prefix for consistency and clarity.Data Structure Improvements
omitemptyto theWarrantyExpWarningPeriodfield inDeviceDatato prevent sending null values in JSON payloads.Miscellaneous
stringsandutilsin relevant files to support new functionality. [1] [2] [3]